-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-12732] - fix new send button #11266
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #11266 +/- ##
==========================================
+ Coverage 33.20% 33.22% +0.02%
==========================================
Files 2727 2730 +3
Lines 85175 85257 +82
Branches 16204 16221 +17
==========================================
+ Hits 28279 28326 +47
- Misses 54643 54675 +32
- Partials 2253 2256 +3 ☔ View full report in Codecov by Sentry. |
Fixed Issues
|
get iconClass() { | ||
return this.showIcon ? "bwi bwi-plus-f" : null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why have this getter if the *ngIf
within the mark up can already hide show the icon?
@@ -1,7 +1,7 @@ | |||
<popup-page> | |||
<popup-header slot="header" [pageTitle]="'send' | i18n"> | |||
<ng-container slot="end"> | |||
<tools-new-send-dropdown *ngIf="!sendsDisabled"></tools-new-send-dropdown> | |||
<tools-new-send-dropdown [showIcon]="true" *ngIf="!sendsDisabled"></tools-new-send-dropdown> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer an optional hideIcon
as the more common usage is the + New
instead of New Send
. Considering the future usage within web and potentially desktop
Which would mean instead of changing line 4, you'd need to modify line 26
@@ -1,6 +1,6 @@ | |||
<button bitButton [bitMenuTriggerFor]="itemOptions" buttonType="primary" type="button"> | |||
<i class="bwi bwi-plus-f" aria-hidden="true"></i> | |||
{{ "new" | i18n }} | |||
<i *ngIf="iconClass" class="{{ iconClass }}" aria-hidden="true"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class being filled by the getter iconClass is unnecessary as it can always be bwi bwi-plus-f
<i class="bwi bwi-plus-f" aria-hidden="true"></i> | ||
{{ "new" | i18n }} | ||
<i *ngIf="iconClass" class="{{ iconClass }}" aria-hidden="true"></i> | ||
{{ (iconClass ? "new" : "createSend") | i18n }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can then also be achieved via the hideIcon
input. Modifying the text, using the same input/getter for the icon is not ideal and my suggestion doesn't improve that. I'm open to ideas, but also fine with keeping it as suggested
@djsmith85 Agree 💯% with your remarks. This was overly complicated for such a simple implementation. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12732
📔 Objective
This PR fixes the create send button shown in the main window when no sends are active.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes